Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Ad coefficient update for last seen ad #24253

Merged
merged 2 commits into from
Jun 25, 2024
Merged

Ad coefficient update for last seen ad #24253

merged 2 commits into from
Jun 25, 2024

Conversation

ptjames
Copy link
Contributor

@ptjames ptjames commented Jun 17, 2024 •
edited by kjozwiak
Loading

This PR updates the default value of last_seen_ad_predictor_weight to 0. Recent AB testing demonstrates this updates leads to a significant increase in CTR.

Resolves brave/brave-browser#39326

Submitter Checklist:

  • I confirm that no security/privacy review is needed and no other type of reviews are needed, or that I have requested them
  • There is a ticket for my issue
  • Used Github auto-closing keywords in the PR description above
  • Wrote a good PR/commit description
  • Squashed any review feedback or "fixup" commits before merge, so that history is a record of what happened in the repo, not your PR
  • Added appropriate labels (QA/Yes or QA/No; release-notes/include or release-notes/exclude; OS/...) to the associated issue
  • Checked the PR locally:
    • npm run test -- brave_browser_tests, npm run test -- brave_unit_tests wiki
    • npm run presubmit wiki, npm run gn_check, npm run tslint
  • Ran git rebase master (if needed)

Reviewer Checklist:

  • A security review is not needed, or a link to one is included in the PR description
  • New files have MPL-2.0 license header
  • Adequate test coverage exists to prevent regressions
  • Major classes, functions and non-trivial code blocks are well-commented
  • Changes in component dependencies are properly reflected in gn
  • Code follows the style guide
  • Test plan is specified in PR before merging

After-merge Checklist:

Test Plan:

@ptjames ptjames requested a review from a team as a code owner June 17, 2024 20:54
tmancey
tmancey requested changes Jun 17, 2024
View reviewed changes
Copy link
Collaborator

@tmancey tmancey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unit tests will fail (or they should do). Can you please update the unit tests too. Thanks

@ptjames ptjames changed the title Coefficient update for last seen ad Notification ad coefficient update for last seen ad Jun 17, 2024
@tmancey
Copy link
Collaborator

tmancey commented Jun 18, 2024 •
edited
Loading

@ptjames can you please add a link to what issue this resolves and a test plan if this requires QA, see description, thanks

@ptjames ptjames changed the title Notification ad coefficient update for last seen ad Ad coefficient update for last seen ad Jun 20, 2024
@ptjames ptjames force-pushed the recency_coefficients_update branch from e6448e5 to 07643e8 Compare June 20, 2024 20:13
@ptjames ptjames force-pushed the recency_coefficients_update branch from 02e286f to edd5f5d Compare June 24, 2024 14:46
@ptjames ptjames requested a review from tmancey June 24, 2024 16:00
tmancey
tmancey approved these changes Jun 24, 2024
View reviewed changes
Copy link
Collaborator

@tmancey tmancey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM++

@tmancey
Copy link
Collaborator

tmancey commented Jun 24, 2024 •
edited
Loading

@ptjames approved. Could you please raise an issue, link to it in the description, and add a test plan, before merging! Once merged, can you please add max_version to the Griffin seed for the associated experiment so that the experiment is removed after ~1 year. Thank you very much.

@tmancey tmancey self-requested a review June 24, 2024 21:19
@tmancey
Copy link
Collaborator

tmancey commented Jun 24, 2024

@ptjames could you please squash any fix-up commits. Thanks

tmancey
tmancey approved these changes Jun 25, 2024
View reviewed changes
Copy link
Collaborator

@tmancey tmancey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM++

@ptjames
coefficient update for last seen ad
b783148 
unit tests updated

Fix tests

unit tests updated

unit tests updated
@ptjames
coefficient update for last seen ad
6708797 
unit tests updated

Fix tests

unit tests updated

unit tests updated

cleaning up unit tests
@ptjames ptjames force-pushed the recency_coefficients_update branch from edd5f5d to 6708797 Compare June 25, 2024 15:20
@github-actions GitHub Actions
Copy link
Contributor

[puLL-Merge] - brave/brave-core@24253

Description

This PR makes several changes to the ad serving and prediction models in the Brave Ads system. The main changes include:

  1. Updating the ad targeting logic to use a hierarchical segment structure.
  2. Modifying the scoring system for ad relevance, particularly by reducing the weight of the "last seen ad" factor.
  3. Adjusting unit tests to reflect these changes in targeting and scoring.

The motivation for these changes appears to be improving the relevance and effectiveness of ad targeting while simplifying some aspects of the prediction model.

Changes

Changes

  1. eligible_inline_content_ads_v2_unittest.cc, eligible_new_tab_page_ads_v2_unittest.cc, eligible_notification_ads_v2_unittest.cc:

    • Updated test cases to use a hierarchical segment structure (e.g., "parent", "parent-child").
    • Modified expectations for ad selection based on the new targeting logic.

  2. creative_ad_model_based_predictor_feature.h files:

    • Changed the default weight for the "last seen ad" predictor from 1.0 to 0.0 across different ad types.

  3. creative_ad_model_based_predictor_last_seen_input_variable_info.h:

    • Changed the default weight for the "last seen ad" input variable from 1.0 to 0.0.

  4. Various *_unittest.cc files:

    • Updated test expectations to reflect the changes in scoring and weighting, particularly the reduced influence of the "last seen ad" factor.

  5. creative_ad_model_based_predictor_scoring_unittest.cc:

    • Adjusted expected scores based on the new weighting system.

Possible Issues

  1. The significant reduction in the weight of the "last seen ad" factor (from 1.0 to 0.0) might lead to more frequent repetition of ads for users. This could potentially impact user experience if not carefully managed.

  2. The changes to the targeting logic using hierarchical segments may require updates to the ad catalog or segmentation system to ensure compatibility.

Security Hotspots

No significant security issues were identified in this change. The modifications primarily affect internal ad serving logic and do not appear to introduce new attack vectors or expose sensitive information.

@ptjames ptjames merged commit 0f6ee64 into master Jun 25, 2024
19 checks passed
@ptjames ptjames deleted the recency_coefficients_update branch June 25, 2024 18:53
@github-actions github-actions bot added this to the 1.69.x - Nightly milestone Jun 25, 2024
This was referenced Jul 8, 2024
Merged
Merged
Open
@kjozwiak kjozwiak mentioned this pull request Jul 9, 2024
Closed
kjozwiak pushed a commit to brave/brave-variations that referenced this pull request Jul 9, 2024
@ptjames
Updating most performant test leg of ad model predictor recency study (…
c8d8b3e 
…#1113)

Updating most performant test leg of ad model predictor recency study to
100% until brave-core changes reach production.

Issue: #1114
Changes become permanent in brave-core via this PR:
brave/brave-core#24253
kjozwiak added a commit to brave/brave-variations that referenced this pull request Jul 10, 2024
@ptjames @kjozwiak
Updating most performant test leg of ad model predictor recency study (…
9821038 
…#1115)

Updating most performant test leg of ad model predictor recency study to
100% until brave-core changes reach production.

Issue: #1114
Changes become permanent in brave-core via this PR:
brave/brave-core#24253

Co-authored-by: Kamil Jozwiak <kamiljoz@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tmancey tmancey tmancey approved these changes

Assignees

@ptjames ptjames

Labels
puLL-Merge
Projects
None yet
Milestone
1.69.x - Release
Development

Successfully merging this pull request may close these issues.

[ads] Update ad serving function last seen coefficient to zero
2 participants
@ptjames @tmancey